FIX: Pen touch input triggers UI/Click action two times (ISXB-1456) #2201
+122
−32
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2201 +/- ##
===========================================
- Coverage 68.11% 68.10% -0.02%
===========================================
Files 367 367
Lines 53610 53603 -7
===========================================
- Hits 36519 36506 -13
- Misses 17091 17097 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jfreire-unity
force-pushed
the
isxb-1456-fix-pen-touch-double-ui-clicks
branch
from
fb31fe7 to
96b6291
Compare
This fixes ISXB-687 and adds a test.
jfreire-unity
force-pushed
the
isxb-1456-fix-pen-touch-double-ui-clicks
branch
from
bd091b8 to
4343463
Compare
There was a problem hiding this comment.
Pull Request Overview
A fix for double-firing UI click events when pen and touch input arrive simultaneously.
- Updated pointer removal logic to handle empty state lists and only decrement indices when removal actually occurs.
- Simplified filtering to immediately remove non–mouse/pen pointers when a mouse or pen event is detected.
- Expanded UITests with cleanup steps, simultaneous mouse+touch scenarios, and a test for disabling the UI module during a click.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Packages/com.unity.inputsystem/.../InputSystemUIInputModule.cs | Reordered empty-state check, moved the assert, removed legacy touch-delay logic, and refined removal in Process. |
| Assets/Tests/InputSystem/Plugins/UITests.cs | Added touch cleanup, new simultaneous-input tests, and a disabling-module-during-click test. |
Comments suppressed due to low confidence (3)
Assets/Tests/InputSystem/Plugins/UITests.cs:1536
- [nitpick] The method name ends with a duplicate "Works"; consider renaming to improve clarity (e.g.,
UI_DisablingEventSystemOnClickWorksWithTouchPointers).
public IEnumerator UI_DisablingEventSystemOnClickEventWorksWithTouchPointersWorks()
Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs:2075
- Consider validating that the
indexparameter is also less thanm_PointerStates.lengthbefore accessingm_PointerStates[index]to avoid potential out-of-range errors.
if (m_PointerStates.length == 0)
Assets/Tests/InputSystem/Plugins/UITests.cs:1464
- The same touch ID (
1) is used for ending touches on different devices; ensure eachEndTouchcall uses the matching ID from its correspondingBeginTouch.
EndTouch(1, secondPosition, screen: touch1);
| if (state.pointerType == UIPointerType.Touch && (state.leftButton.isPressed || state.leftButton.wasReleasedThisFrame)) | ||
| { | ||
| // The pointer was not removed | ||
| // Pointers can have be reset before (e.g. when calling OnDisable) which would make m_PointerStates |
There was a problem hiding this comment.
Grammar: "have be reset" should be "have been reset".
Suggested change
| // Pointers can have be reset before (e.g. when calling OnDisable) which would make m_PointerStates | |
| // Pointers can have been reset before (e.g. when calling OnDisable) which would make m_PointerStates |
Copilot uses AI. Check for mistakes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Please fill this section with a description what the pull request is trying to address and what changes were made.
Testing status & QA
Please describe the testing already done by you and what testing you request/recommend QA to execute. If you used or created any testing project please link them here too for QA.
Overall Product Risks
Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: